PADV-1557 Adjust storage backend definition via site configurations #9

Nekenhei · 2024-11-17T17:13:56Z

Tickets

https://agile-jira.pearson.com/browse/PADV-1557

Description

This PR adjust the upstream feature of defining storage backends but make it site-aware by defining it via site configurations.

Changes Made

Adjust backend definition pull from site helpers.
Adjust test cases
Add feature documentation

How To Test

Please follow Xblock's doc to use the SGA as it used to work, nothing shall change in terms of user experience when uploading or downloading files.
Through the site configuration, set the following settings format using the keys provided by AWS S3 for a given bucket.

"SGA_STORAGE_SETTINGS": {
    "STORAGE_CLASS": "your.desire.storage.backend,
    "STORAGE_KWARGS":  {
        "key": "value"
    }
}

Files shall be processed thru the desire storage.

Annotations

Broken dependencies and CI process

CI process of unit testing and linting is failing due to dependencies issues. It also restrict the possibility to run it locally despite of the presence of a Tox defined workflow to do so. This is because it expects the workflows and credentials from upstream, not pearson. That could be cover in future PRs, but is not the purpose of this one so it won't be reviewed.

Unit Testing

As per the missing dependencies and challenges with Tox definition, the unit testing shall be executed thru the paltaform shell. More documentation about it can be found here.

Squirrel18 · 2024-11-18T15:44:03Z

docs/001-custom-features.md

+### Context
+
+The SGA Xblock in its upstream repository approaches the storage capability by relaying on the Django's default storage
+that is defined in the edx platform. Tehre's also a feature where the storage backend to be used can be defined via


Suggested change

that is defined in the edx platform. Tehre's also a feature where the storage backend to be used can be defined via

that is defined in the edx platform. There's also a feature where the storage backend to be used can be defined via

Squirrel18 · 2024-11-18T15:45:36Z

docs/001-custom-features.md

+
+### Feature
+
+To define a desire backend shall follow the following format by defining the storage class and it's kwargs.


Suggested change

To define a desire backend shall follow the following format by defining the storage class and it's kwargs.

To define a desired backend shall follow the following format by defining the storage class and its kwargs.

Squirrel18 · 2024-11-18T15:45:58Z

docs/001-custom-features.md

+}
+
+Once it has been done, the SGA xblocks for the given site will manage the media objects with the defined storage backend.
+If no settings are defined, the Xblock would use the default Django storage


Suggested change

If no settings are defined, the Xblock would use the default Django storage

If no settings are defined, the Xblock will use the default Django storage.

Squirrel18 · 2024-11-18T15:46:23Z

edx_sga/tests/test_utils.py

+                    "region_name": "us-east-1"
+                }
+            }
+        }


Trailing comma. @Nekenhei

Squirrel18 · 2024-11-18T17:11:06Z

docs/001-custom-features.md

+}
+
+Once it has been done, the SGA xblocks for the given site will manage the media objects with the defined storage backend.
+If no settings are defined, the Xblock will use the default Django storage


Suggested change

If no settings are defined, the Xblock will use the default Django storage

If no settings are defined, the Xblock will use the default Django storage.

Squirrel18 · 2024-11-18T17:11:16Z

docs/001-custom-features.md

+
+### Feature
+
+To define a d backend shall follow the following format by defining the storage class and its kwargs.


Suggested change

To define a d backend shall follow the following format by defining the storage class and its kwargs.

To define a backend shall follow the following format by defining the storage class and its kwargs.

Squirrel18 · 2024-11-18T17:11:38Z

edx_sga/tests/test_utils.py

+                    "region_name": "us-east-1"
+                }
+            }


Trailing comma. @Nekenhei

Squirrel18 · 2024-11-19T00:54:06Z

edx_sga/tests/test_utils.py

+                }
+            }


Trailing comma. @Nekenhei

Pylint isn't notifying about the needs of this trailing comma, also, this shouldn't change in time since is a unit test focused exclusively on that scenario so no further changes on the dict shall exists. Nevertheless I understand that this is a good practice and should be applied anytime we can do so.

Squirrel18 · 2024-11-19T00:54:19Z

docs/001-custom-features.md

+
+### Feature
+
+To define a desire backend, follow the following format by defining the storage class and its kwargs.


Suggested change

To define a desire backend, follow the following format by defining the storage class and its kwargs.

To define a desired backend, follow the following format by defining the storage class and its kwargs.

This is weird, I know you've pointed this two times and I was pretty sure I'd push it to the PR. But seems it didn't happen. Comment applied.

Squirrel18 · 2024-11-19T15:25:30Z

edx_sga/tests/test_utils.py

+                    "secret_key": "test_secret",
+                    "bucket_name": "test-bucket-1",
+                    "region_name": "us-east-1",
+                }


Trailing coma. @Nekenhei

Hi @Squirrel18 according to the the PEP 8, the trailing comma is an optional standard and is recommend on those version controls where the dict is expected to change over time. Also, Pylint ain't suggesting to apply so.

I understand that is a good practice to apply the trailing commas whenever we can because there are some changes that we cannot foresee. Nevertheless, that's why on my previous response I've explained why I haven't use it. It's because this is a unit test case where no changes shall exists on the dict definitions unless the feature itself changes and in consequence the whole dict in its own format and structure will change.

I've applied the change on that line, this is comment is only to deeply explain why I haven't take those in mind during the development of the test scenario (upstream doesn't have it too).

We have defined it as good practice to apply trailing commas regardless of the case following PEP8.
Trailing commas have nothing to do with the fact that you don't expect the list to grow, trailing commas are a convention for version control systems, not for unit tests, so applying trailing commas does not depend on the test or any other implementation.
We have different practices than upstream, so we should follow our own practices and not depend on others.

Nekenhei requested a review from Squirrel18 November 18, 2024 12:21

Squirrel18 requested changes Nov 18, 2024

View reviewed changes

Squirrel18 requested changes Nov 19, 2024

View reviewed changes

Squirrel18 approved these changes Nov 19, 2024

View reviewed changes

feat: site-aware storage backend definition

65c02a4

Nekenhei force-pushed the vue/PADV-1557 branch from 64c9fbf to 65c02a4 Compare November 19, 2024 16:34

Nekenhei merged commit d970cb1 into pearson-release/olive.main Nov 19, 2024
0 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PADV-1557 Adjust storage backend definition via site configurations #9

PADV-1557 Adjust storage backend definition via site configurations #9

Nekenhei commented Nov 17, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 18, 2024

Squirrel18 Nov 19, 2024

Nekenhei Nov 19, 2024

Squirrel18 Nov 19, 2024

Nekenhei Nov 19, 2024

Squirrel18 Nov 19, 2024

Nekenhei Nov 19, 2024

Squirrel18 Nov 19, 2024

	that is defined in the edx platform. Tehre's also a feature where the storage backend to be used can be defined via
	that is defined in the edx platform. There's also a feature where the storage backend to be used can be defined via


		### Feature

		To define a desire backend shall follow the following format by defining the storage class and it's kwargs.

	To define a desire backend shall follow the following format by defining the storage class and it's kwargs.
	To define a desired backend shall follow the following format by defining the storage class and its kwargs.

	If no settings are defined, the Xblock would use the default Django storage
	If no settings are defined, the Xblock will use the default Django storage.


		### Feature

		To define a d backend shall follow the following format by defining the storage class and its kwargs.

	To define a d backend shall follow the following format by defining the storage class and its kwargs.
	To define a backend shall follow the following format by defining the storage class and its kwargs.


		### Feature

		To define a desire backend, follow the following format by defining the storage class and its kwargs.

	To define a desire backend, follow the following format by defining the storage class and its kwargs.
	To define a desired backend, follow the following format by defining the storage class and its kwargs.

PADV-1557 Adjust storage backend definition via site configurations #9

PADV-1557 Adjust storage backend definition via site configurations #9

Conversation

Nekenhei commented Nov 17, 2024

Tickets

Description

Changes Made

How To Test

Annotations

Broken dependencies and CI process

Unit Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment